Skip to content

Prevent .tar file corruption by patching short reads#261

Open
joost-j wants to merge 5 commits intofox-it:mainfrom
joost-j:bugfix/pad-shrunk-file-in-tar-output
Open

Prevent .tar file corruption by patching short reads#261
joost-j wants to merge 5 commits intofox-it:mainfrom
joost-j:bugfix/pad-shrunk-file-in-tar-output

Conversation

@joost-j
Copy link
Contributor

@joost-j joost-j commented Aug 12, 2025

Fixes an issue where a .tar output file would contain inconsistencies with regards to expected and actual file size of the included files.

In some cases, a file on disk can report a size of X bytes, but at the time of actually reading X bytes from the file, less than X bytes are actually available in the file (a short read). Acquire would report these issues as OSError in the resulting Acquisition log file, because the Python stdlib tarfile.py handles it that way. Data may however already be written to the destination archive at that point.

Afterwards, Acquire continues to add new files to the archive. When trying to untar the file using tar -xvf <FILE> this would show as a tar: Skipping to next header error and finally, the process exists with a nonzero exit code.

Included a test case which simulates a file that actually returns less bytes than its reported size, to test this case.

@codecov
Copy link

codecov bot commented Aug 12, 2025

Codecov Report

❌ Patch coverage is 86.48649% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.17%. Comparing base (2956b06) to head (4373d8f).

Files with missing lines Patch % Lines
acquire/outputs/tar.py 86.48% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
+ Coverage   44.75%   45.17%   +0.41%     
==========================================
  Files          26       26              
  Lines        3546     3582      +36     
==========================================
+ Hits         1587     1618      +31     
- Misses       1959     1964       +5     
Flag Coverage Δ
unittests 45.17% <86.48%> (+0.41%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joost-j joost-j requested a review from Schamper August 12, 2025 14:39
for _ in range(blocks):
# Prevents "long reads" because it reads at max bufsize bytes at a time
buf = fh.read(bufsize)
if len(buf) < bufsize:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can generalize this case instead of doing it twice. Keep track of how many bytes you actually wrote (i.e. using .tell() and only pad once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know exactly what you mean by this, since it looks like we need to calculate the remainder for two different sizes (bufsize vs tarfile.BLOCKSIZE). But please do change / refactor the code if you can write this more concise.


info = copy.copy(info)

buf = info.tobuf(self.tar.format, self.tar.encoding, self.tar.errors)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make this even safer by truncating to the previous offset/tar member end if any exception occurs while writing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, added a try/except around it to restore back to the original offset, plus a test case.

@qmadev
Copy link
Contributor

qmadev commented Sep 9, 2025

Any idea when this is getting fixed? It's affecting me as well. Let me know if there's anything I can do to help!

@joost-j joost-j force-pushed the bugfix/pad-shrunk-file-in-tar-output branch from 00a211c to e990898 Compare February 27, 2026 14:17
@joost-j joost-j force-pushed the bugfix/pad-shrunk-file-in-tar-output branch from e990898 to 186a3e6 Compare March 11, 2026 08:31
@joost-j joost-j requested a review from Schamper March 11, 2026 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants